Add serde for ColumnBasedRowsAndColumns to fix window queries without group by #16658
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kgyrtkirk
reviewed
Jun 28, 2024
...rc/main/java/org/apache/druid/query/rowsandcols/concrete/ColumnBasedFrameRowsAndColumns.java
Outdated
Show resolved
Hide resolved
kgyrtkirk
reviewed
Jun 28, 2024
sql/src/test/resources/calcite/tests/window/wikipediaFramedAggregations.sqlTest
Show resolved
Hide resolved
kgyrtkirk
reviewed
Jun 28, 2024
kgyrtkirk
reviewed
Jun 28, 2024
...est/java/org/apache/druid/query/rowsandcols/concrete/ColumnBasedFrameRowsAndColumnsTest.java
Outdated
Show resolved
Hide resolved
kgyrtkirk
reviewed
Sep 5, 2024
processing/src/main/java/org/apache/druid/query/rowsandcols/concrete/FrameRowsAndColumns.java
Outdated
Show resolved
Hide resolved
Comment on lines
162
to
163
| false, | ||
| ByteBuffer.allocate(Frame.compressionBufferSize((int) frame.numBytes())), |
There was a problem hiding this comment.
compression is false; meanwhile a compressionBuffer is being allocated at every call - is that required?
if its needed - would it be possible to reuse the buffer later?
There was a problem hiding this comment.
Since we are not trying to compress, I made the buffer null
Lets discuss on this, If compression can improve the performance, I can work on follow-up PR to do that work
processing/src/main/java/org/apache/druid/jackson/DruidDefaultSerializersModule.java
Show resolved
Hide resolved
kgyrtkirk
reviewed
Sep 5, 2024
| ) | ||
| ); | ||
|
|
||
| ColumnBasedFrameRowsAndColumns frc = ColumnBasedFrameRowsAndColumnsTest.buildFrame(input); |
There was a problem hiding this comment.
I was thinking if this creation could be generailzed by adding it to RowsAndColumnsTestBase#MAKERS - or that's not really usefull?
sreemanamala
commented
Sep 9, 2024
processing/src/main/java/org/apache/druid/query/rowsandcols/LazilyDecoratedRowsAndColumns.java
Outdated
Show resolved
Hide resolved
kgyrtkirk
reviewed
Sep 9, 2024
processing/src/main/java/org/apache/druid/query/rowsandcols/RowsAndColumns.java
Outdated
Show resolved
Hide resolved
kgyrtkirk
reviewed
Sep 9, 2024
processing/src/main/java/org/apache/druid/query/rowsandcols/RowsAndColumns.java
Outdated
Show resolved
Hide resolved
kgyrtkirk
reviewed
Sep 11, 2024
processing/src/test/java/org/apache/druid/common/semantic/SemanticCreatorUsageTest.java
Outdated
Show resolved
Hide resolved
kgyrtkirk
reviewed
Sep 11, 2024
| @@ -16086,6 +16086,7 @@ public void testScanAndSortOnJoin() | |||
| .run(); | |||
| } | |||
|
|
|||
| @NotYetSupported(Modes.UNSUPPORTED_DATASOURCE) | |||
There was a problem hiding this comment.
hmm.. I wonder how did this started to happen?
its not a blocker just interested
There was a problem hiding this comment.
Not very sure though, but I expected this was because we used to send empty to window operator query and that has been changed now. Let me check the actual difference and comment back.
kgyrtkirk
approved these changes
Sep 16, 2024
processing/src/main/java/org/apache/druid/query/rowsandcols/concrete/FrameRowsAndColumns.java
Outdated
Show resolved
Hide resolved
10 tasks
pranavbhole
pushed a commit
to pranavbhole/druid
that referenced
this pull request
Sep 17, 2024
… group by (apache#16658) Register a Ser-De for RowsAndColumns so that the window operator query running on leaf operators would be transferred properly on the wire. Would fix the empty response given by window queries without group by on the native engine.
sreemanamala
added a commit
to sreemanamala/druid
that referenced
this pull request
Sep 18, 2024
… group by (apache#16658) Register a Ser-De for RowsAndColumns so that the window operator query running on leaf operators would be transferred properly on the wire. Would fix the empty response given by window queries without group by on the native engine.
10 tasks
sreemanamala
added a commit
to sreemanamala/druid
that referenced
this pull request
Sep 18, 2024
… group by (apache#16658) Register a Ser-De for RowsAndColumns so that the window operator query running on leaf operators would be transferred properly on the wire. Would fix the empty response given by window queries without group by on the native engine. (cherry picked from commit bb1c3c1)
sreemanamala
added a commit
to sreemanamala/druid
that referenced
this pull request
Sep 19, 2024
… group by (apache#16658) Register a Ser-De for RowsAndColumns so that the window operator query running on leaf operators would be transferred properly on the wire. Would fix the empty response given by window queries without group by on the native engine. (cherry picked from commit bb1c3c1)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Register a Ser-De for RowsAndColumns so that the window operator query running on leaf operators would be transferred properly on the wire. Would fix the empty response given by window queries without group by on the native engine.
Key changed/added classes in this PR
RowsAndColumnsFrameRowsAndColumnsDruidDefaultSerializersModuleThis PR has: